fix(cli): allow multiple provider profiles to be deleted - #3032
Conversation
Signed-off-by: Gordon Sim <gsim@redhat.com>
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
The batch loop still exits on the first RPC error because .into_diagnostic()? is inside the loop. For provider profiles, a missing, source-managed, or in-use profile returns an RPC error, so later IDs are never attempted.
This matches the current provider delete fail-fast behavior, but it conflicts with this issue's acceptance criteria to attempt each supplied profile and report each result independently. Could we continue through all IDs, print each failure, and return an aggregated nonzero error afterward? Please also add a mixed-result test such as success, in-use or missing, then success.
Follow-up: sandbox delete, provider delete, and workspace delete also stop on the first RPC error. Please consider a separate issue to define and standardize batch-delete error semantics across the CLI, including per-target reporting and aggregate exit status.
|
@elezar I propose to reword the acceptance criteria for the associated issue, then open a separate issue and PR for improving the error handling of deletion across all three resource types. Is that good with you? |
That's fine. |
New issue: #3102 (The acceptance criteria for this issue has also been updated) |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
This focused CLI change is project-valid through #3029, and the initial code review found no blocking defects. Thanks @elezar and @grs for clarifying that cross-command batch error aggregation belongs in #3102; I checked this patch against the updated, intentionally fail-fast acceptance criteria.
Blocking findings:
- No blocking findings remain
Carried findings:
- None
Non-blocking suggestions:
- Add a short Fern example such as
openshell provider profile delete custom-api custom-alttodocs/sandboxes/providers-v2.mdxso users can discover the new multi-ID form.
Gator metadata
- Validation: Focused CLI consistency improvement linked to validated issue #3029
- Docs: Agent CLI reference updated; a Fern usage example is suggested
- Checks: DCO and vouch are green; current-head Branch Checks and Helm Lint dispatch is still pending
- E2E: Not required for this CLI argument-cardinality change
- Head SHA:
adc08d913b3b1149ac8a6f9a0613dd10cb424358 - Base SHA:
69a05ebb3b154e304a66fe80eed8504e889abc6d - Merge base SHA:
69a05ebb3b154e304a66fe80eed8504e889abc6d - Patch ID:
a59218c808bc1fe35c787de9dc1f703aae709106 - Gator payload:
7 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
johntmyers
added
the
gator:in-review
|
/ok to test adc08d9 |
johntmyers
added
gator:watch-pipeline
|
@grs see above:
Please add this to the follow on PR that handles robust iteration when some deletions fail, we also will want to describe that behavior in the docs updates. |
Will do! |
johntmyers
added
gator:approval-needed
Monitoring CompleteMonitoring is complete because this PR has merged. Final status: Gator completed code review with no blocking findings, the current-head required checks were green, and maintainer approval was present before merge. I removed the active Gator metadata
|
johntmyers
removed
the
gator:approval-needed
3 participants
Summary
Makes provider profile deletion more consistent with that of sandboxes and profiles by allowing the deletion of multiple items.
Related Issue
Closes #3029
Changes
provider profile deletecommand.Testing
mise run pre-commitpassesChecklist